WIP/POC: handle sidecar containers #728

dicarlo2 · 2019-04-04T19:59:08Z

Changes

Please see #727 for the proposal for this POC.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide
for more details.

Release Notes

release-note

tekton-robot · 2019-04-04T19:59:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dicarlo2
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: vdemeester

If they are not already assigned, you can assign the PR to them by writing /assign @vdemeester in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

abayer · 2019-04-04T20:29:50Z

/ok-to-test

woo! I've been looking forward to something along these lines.

dicarlo2 · 2019-04-04T20:39:28Z

Oh, yea, forgot to mention, not only are there no tests but I haven't even looked at the ones that already exist (and are likely failing). If we're happy with this approach I can clean up the PR + fix/add tests.

bobcatfish · 2019-04-25T18:19:54Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+	ts.Volumes = append(ts.Volumes, corev1.Volume{
+		Name: entrypoint.DownwardMountName,
+		VolumeSource: corev1.VolumeSource{
+			DownwardAPI: &corev1.DownwardAPIVolumeSource{


i have a naiive question here about DownwardAPIVolumeSource, i tried to find it in the kube docs but failed: when this volume is mounted, is the content of the volume kept up to date, or is it snapshotted at mount time only? (and if it is kept up to date, how is that done? possibly relevant to our Pull Request resource plans... 😇 )

It's kept up to date, and afaik the kubelet handles keeping it up to date for each pod on the host by just watching the pod spec and updating appropriately.

ah interesting, good to know!

bobcatfish · 2019-04-25T18:28:01Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

 	}
+
+	sidecars := len(pod.Status.ContainerStatuses) - len(taskRun.Status.Steps)
+	if pod.Status.Phase == corev1.PodRunning && sidecarsReady == sidecars {


do you think we should worry at all about a state where a sidecar container's status indicates it is "ready", but the underlying binary isn't actually ready? (e.g. the binary is running but not actually ready to handle requests)

This approach effectively defines an API/contract for users to follow to signal to tekton that their sidecars are ready, that is, it's defined by the readiness probe of the sidecar. I think that if a use-case falls outside of the ability to be defined by a readiness probe, which can be arbitrary commands/scripts, then it's probably too complex for there to be a way to specify it inside of tekton.

Within user's steps they're free to implement any additional logic they need for more complex ready checks, this really only impacts sidecars which affect automatically specified steps (e.g. git-init).

I think we wait until there's a clear use-case that readiness probes don't satisfy before coming up with additional ways for user's to specify readiness.

Makes sense - sounds great :D

bobcatfish · 2019-04-25T18:37:07Z

After our back and forth on #727 I think this approach is pretty slick, if you want to continue to flesh it out @dicarlo2 !

For next steps I think this PR in particular would need docs, tests, etc. (which I think you already know, totally understand that you wanted feedback first). Would be great to discuss at our next working group meeting on Tuesday too if you are able to join.

Anyway this seems great, let's do it! 😎

dicarlo2 · 2019-05-04T00:45:22Z

Would be great to discuss at our next working group meeting on Tuesday too if you are able to join.

I've put it on my schedule to attend the next one (this coming Tuesday).

bobcatfish · 2019-05-09T19:43:54Z

I've put it on my schedule to attend the next one (this coming Tuesday).

I think I let you down here, I didn't add this to the agenda 🤦‍♀ (btw feel free to edit the agenda directly yourself also!) but I have NOW for the next meeting, which is Tuesday May 14 (but I won't be there unfortunately 😩 ) - the following week will be cancelled for Kubecon (any chance you'll be at kubecon?)

But long story short @dicarlo2 I think we should go ahead with this! And probably @sbwsg will need/want this for the work he's doing on log streaming :D

@dicarlo2 would you be open to having one of us continue the PR, e.g. adding some more tests, docs, the stuff we need to get this ready to merge? of course if you want to take that on that's totally cool too :D (and apologies this back and forth has taken so long!)

dicarlo2 · 2019-05-13T16:50:33Z

@bobcatfish feel free to take over, I've been jumping back and forth between a few projects so I haven't had as much time as I would have liked to focus on this.

tekton-robot · 2019-05-22T15:15:50Z

@dicarlo2: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
tekton-pipeline-unit-tests	`ad4890c`	link	`/test tekton-pipeline-unit-tests`
pull-tekton-pipeline-unit-tests	`15659a4`	link	`/test tekton-pipeline-unit-tests`
pull-tekton-pipeline-build-tests	`15659a4`	link	`/test pull-tekton-pipeline-build-tests`
pull-tekton-pipeline-integration-tests	`15659a4`	link	`/test pull-tekton-pipeline-integration-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

dicarlo2 · 2019-05-22T15:26:00Z

I think I let you down here, I didn't add this to the agenda 🤦‍♀ (btw feel free to edit the agenda directly yourself also!) but I have NOW for the next meeting, which is Tuesday May 14 (but I won't be there unfortunately 😩 ) - the following week will be cancelled for Kubecon (any chance you'll be at kubecon?)

No, you didn't let me down, as usual my schedule is a mess so I missed the meeting :/

ghost · 2019-05-31T12:30:40Z

I'm closing this PR now that @joseblas and I are working in a shared branch based off of this one. The work can be tracked in the WIP PR here: #936. Jose has done a great job getting the existing tests passing again in that branch.

Add vendor 1.22 patch

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 4, 2019

tekton-robot requested review from bobcatfish and imjasonh April 4, 2019 19:59

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Apr 4, 2019

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 4, 2019

dicarlo2 mentioned this pull request Apr 4, 2019

Proposal: graceful sidecar support #727

Closed

tekton-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 4, 2019

dicarlo2 mentioned this pull request Apr 11, 2019

Remove nop container and image #715

Closed

3 tasks

bobcatfish reviewed Apr 25, 2019

View reviewed changes

todo: add commit message, tests, docs

15659a4

dicarlo2 force-pushed the sidecar_ready branch from ad4890c to 15659a4 Compare May 22, 2019 14:55

ghost mentioned this pull request May 29, 2019

Merge "Graceful Sidecar Support" implementation #926

Closed

6 tasks

ghost self-assigned this May 29, 2019

joseblas mentioned this pull request May 31, 2019

Graceful sidecar support #936

Merged

3 tasks

ghost closed this May 31, 2019

nikhil-thomas pushed a commit to nikhil-thomas/pipeline that referenced this pull request Aug 17, 2021

Merge pull request tektoncd#728 from openshift/add-1_22-patch

a305a51

Add vendor 1.22 patch

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP/POC: handle sidecar containers #728

WIP/POC: handle sidecar containers #728

dicarlo2 commented Apr 4, 2019

tekton-robot commented Apr 4, 2019

abayer commented Apr 4, 2019

dicarlo2 commented Apr 4, 2019

bobcatfish Apr 25, 2019

dicarlo2 May 4, 2019

bobcatfish May 9, 2019

bobcatfish Apr 25, 2019

dicarlo2 May 4, 2019

bobcatfish May 9, 2019

bobcatfish commented Apr 25, 2019

dicarlo2 commented May 4, 2019

bobcatfish commented May 9, 2019

dicarlo2 commented May 13, 2019

tekton-robot commented May 22, 2019

dicarlo2 commented May 22, 2019

ghost commented May 31, 2019

WIP/POC: handle sidecar containers #728

WIP/POC: handle sidecar containers #728

Conversation

dicarlo2 commented Apr 4, 2019

Changes

Submitter Checklist

Release Notes

tekton-robot commented Apr 4, 2019

abayer commented Apr 4, 2019

dicarlo2 commented Apr 4, 2019

bobcatfish Apr 25, 2019

Choose a reason for hiding this comment

dicarlo2 May 4, 2019

Choose a reason for hiding this comment

bobcatfish May 9, 2019

Choose a reason for hiding this comment

bobcatfish Apr 25, 2019

Choose a reason for hiding this comment

dicarlo2 May 4, 2019

Choose a reason for hiding this comment

bobcatfish May 9, 2019

Choose a reason for hiding this comment

bobcatfish commented Apr 25, 2019

dicarlo2 commented May 4, 2019

bobcatfish commented May 9, 2019

dicarlo2 commented May 13, 2019

tekton-robot commented May 22, 2019

dicarlo2 commented May 22, 2019

ghost commented May 31, 2019